Add named capturing groups to regexpToRegexp method #225

DamianKu · 2020-06-28T16:47:13Z

I was looking into fixing issues raised on express repository #4277.

I modified regular expression in regexpToRegexp to match not only ( but also (?<*> ^{probably regexp could be better :)}.

No change with normal regexp matching, router.get(/\/regex\/(.+)/) -> localhost/regex/testData will result in {"0":"testData"}.

With named capturing groups, router.get(/\/regex2\/(?<groupname>.+)/) -> localhost/regex2/testData will result in {"groupname":"testData"}.
Mixed matched with named capturing groups, router.get(/\/(.+)\/(?<groupname>.+)\/(.+)/) -> localhost/testRegex/testData/extraStuff" will result in {"0":"testRegex","2":"extraStuff","groupname":"testData"}.

All tests are passing but I didn't add new tests to cover this functionality yet.
Would be great to get some feedback from you guys if I'm even looking in good place or is it worth working on it.

blakeembrey · 2020-07-07T04:24:11Z

This looks really great to me, thanks! From my understanding this will work in all current projects because the named group doesn't remove it from the array of RegExp results (e.g. this code depends on the position of keys in the result:

path-to-regexp/src/index.ts

Line 405 in 98ab888

const key = keys[i - 1];

). If you want to add a couple of tests around it we should be good to merge this.

For getting this feature into the current version of Express, you'll probably need to make this PR against an older branch here: https://github.com/pillarjs/path-to-regexp/blob/0.1.x/index.js

However, I believe this it can land in the next Express.js release with the current implementation.

blakeembrey · 2020-07-07T04:36:41Z

src/index.ts

@@ -453,13 +453,17 @@ export type Token = string | Key;
 function regexpToRegexp(path: RegExp, keys?: Key[]): RegExp {
  if (!keys) return path;

-  // Use a negative lookahead to match only capturing groups.
-  const groups = path.source.match(/\((?!\?)/g);
+  const groups = path.source.match(/\((\?<.*?>)?(?!\?)/g);


Suggested change

const groups = path.source.match(/\((\?<.*?>)?(?!\?)/g);

const groups = path.source.match(/\((?:\?<(.*?)>)?(?!\?)/g);

Something like this will resolve the comment you have below.

Yeah, non-capturing groups, I tried that. Had no luck so I'm guessing, I made some mistake :)

Will try this :)

Something like this will resolve the comment you have below.

That didn't work

$ curl http://localhost:1337/testRegex/testData/extraStuff -w "\n" {"0":"testRegex","2":"extraStuff","(?<groupname>":"testData"}

Oh yeah, that makes sense. It's happening with .match which is only getting the matches and not the groups. We probably need to use regexp.exec in a loop instead.

There's also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/matchAll but it's not supported on earlier node.js versions or browsers.

Using exec worked :)

Looping through exec results doesn't look nice tho.
Any idea how to make it nicer?

DamianKu · 2020-07-08T19:10:20Z

Test is failing on node v8 as Named capture groups are behind '--harmony' runtime flag.

blakeembrey · 2020-07-08T20:53:21Z

@DamianKu You'll probably need to put the tests behind a version flag by checking the node.js version in the test suite.

Use exec instead of match.

DamianKu · 2020-07-11T19:20:50Z

@DamianKu You'll probably need to put the tests behind a version flag by checking the node.js version in the test suite.

@blakeembrey
How I would go about doing that?
Do you have any example in other your projects that I could investigate? :)

blakeembrey · 2020-07-14T01:16:15Z

How I would go about doing that?

You can read the node.js version here: https://nodejs.org/api/process.html#process_process_version. And use something like https://www.npmjs.com/package/semver to compare whether you want to run it. Put it within an if condition to define the test.

Here's an example (albeit using TypeScript): https://github.com/TypeStrong/ts-node/blob/866bce6a175ec124fe62f269b22e91c92b40f875/src/index.spec.ts#L137

DamianKu · 2020-07-14T06:38:54Z

Hah OK 😁 I'm aware of that. I don't know why I was thinking that you meant something more elaborate

…

On Tue, 14 Jul 2020, 02:16 Blake Embrey, ***@***.***> wrote: How I would go about doing that? You can read the node.js version here: https://nodejs.org/api/process.html#process_process_version. And use something like https://www.npmjs.com/package/semver to compare whether you want to run it. Put it within an if condition to define the test. Here's an example (albeit using TypeScript): https://github.com/TypeStrong/ts-node/blob/866bce6a175ec124fe62f269b22e91c92b40f875/src/index.spec.ts#L137 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#225 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEE7BAKDB6CZDWQ7RT4OIT3R3OWW5ANCNFSM4OKTOBNA> .

DamianKu · 2020-07-18T14:30:10Z

I added the semver check after the initialization of TEST variable.

If you prefer I can do ternary in TEST like so:

const TESTS: Test[] = [
    ...,

    ...(gte(process.version, "10.0.0")
    ? [
        [...test...],
        [...test...]
      ]
    : ([] as any)),
    
    ...,
];

blakeembrey · 2020-07-20T02:09:22Z

src/index.ts

+      modifier: "",
+      pattern: ""
+    });
+    index++;


It would probably be good to stay consistent with the behavior in string tokens. In that method, we're only incrementing the counter each non-named group:

path-to-regexp/src/index.ts

Line 189 in 8b0ae94

name: name || key++,

.

That would mean you probably want execResult[1] || index++. We should add a mixed test of named and unnamed groups to make sure it doesn't break.

OK will do that.

Does it mean that this /\/(.+)\/(?<groupname>.+)\/(.+)/ called with /testRegex/testData/extraStuff should result in {"0":"testRegex","1":"extraStuff","groupname":"testData"} ?

…ith non named regexp matching

blakeembrey · 2020-08-10T04:26:31Z

Thanks @DamianKu! This looks great.

Add named capturing groups to regexpToRegexp method

87daaa7

blakeembrey reviewed Jul 7, 2020

View reviewed changes

Add tests for named capturing groups

ba13aa2

Update groups regex to match captured group name.

d8d20f9

Use exec instead of match.

DamianKu added 2 commits July 18, 2020 15:22

Add semver & @types/semver to devDependencies.

c166914

Push named capturing groups tests to test suite only if valid version

9caa26d

mastermatt mentioned this pull request Jul 18, 2020

Capturing groups #229

Closed

blakeembrey reviewed Jul 20, 2020

View reviewed changes

Do not increment index on non capturing groups. Add tests for named w…

5534a33

…ith non named regexp matching

blakeembrey merged commit feddb3d into pillarjs:master Aug 10, 2020

pinksynth mentioned this pull request Jul 22, 2021

Improve documentation #256

Merged

YoutacRandS-VA mentioned this pull request Jun 19, 2024

[Snyk] Upgrade path-to-regexp from 0.1.7 to 6.2.2 YoutacRandS-VA/express#2

Open

abdulrahman305 mentioned this pull request Aug 15, 2024

[Snyk] Upgrade path-to-regexp from 6.1.0 to 6.2.2 abdulrahman305/vercel#99

Closed

rish2497 mentioned this pull request Sep 9, 2024

[Snyk] Upgrade: bcrypt, body-parser, ethers, ethr-did, express, path-to-regexp, module-alias, npm, swagger-ui-express, web-did-resolver, web3, winston, ws i3-Market-V3-Public-Repository/SP3-SSIAM-VerifiableCredentials#65

Open

Omrisnyk mentioned this pull request Sep 10, 2024

[Snyk] Fix for 1 vulnerabilities Omrisnyk/npm-lockfiles#182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add named capturing groups to regexpToRegexp method #225

Add named capturing groups to regexpToRegexp method #225

DamianKu commented Jun 28, 2020

blakeembrey commented Jul 7, 2020

blakeembrey Jul 7, 2020

DamianKu Jul 7, 2020

DamianKu Jul 8, 2020

blakeembrey Jul 8, 2020

DamianKu Jul 11, 2020

DamianKu commented Jul 8, 2020

blakeembrey commented Jul 8, 2020

DamianKu commented Jul 11, 2020

blakeembrey commented Jul 14, 2020

DamianKu commented Jul 14, 2020 via email

DamianKu commented Jul 18, 2020

blakeembrey Jul 20, 2020

DamianKu Jul 25, 2020 •

edited

Loading

blakeembrey commented Aug 10, 2020

	const groups = path.source.match(/\((\?<.*?>)?(?!\?)/g);
	const groups = path.source.match(/\((?:\?<(.*?)>)?(?!\?)/g);

Add named capturing groups to regexpToRegexp method #225

Add named capturing groups to regexpToRegexp method #225

Conversation

DamianKu commented Jun 28, 2020

blakeembrey commented Jul 7, 2020

blakeembrey Jul 7, 2020

Choose a reason for hiding this comment

DamianKu Jul 7, 2020

Choose a reason for hiding this comment

DamianKu Jul 8, 2020

Choose a reason for hiding this comment

blakeembrey Jul 8, 2020

Choose a reason for hiding this comment

DamianKu Jul 11, 2020

Choose a reason for hiding this comment

DamianKu commented Jul 8, 2020

blakeembrey commented Jul 8, 2020

DamianKu commented Jul 11, 2020

blakeembrey commented Jul 14, 2020

DamianKu commented Jul 14, 2020 via email

DamianKu commented Jul 18, 2020

blakeembrey Jul 20, 2020

Choose a reason for hiding this comment

DamianKu Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

blakeembrey commented Aug 10, 2020

DamianKu Jul 25, 2020 •

edited

Loading